[live-update] Modify instance reincarnation to account for sled evacuations - #11504

Does reincarnate_all need to remember why we're reincarnating it? It will kick off start sagas for these instances, but if that saga unwinds, it puts the instance back onto the NoVmm state unconditionally:

I guess that means the real question is whether the instance start saga needs to be told what state to restore if it has to undo?

Yes, I think the ReincarnationReason needs to be passed into the saga. There are a couple other reasons this is necessary:

• As I discussed in https://github.com/oxidecomputer/omicron/pull/11504/changes#r4427704695, we probably don't want to set the time_last_auto_restarted timestamp when the instance-start saga completes, since that will be used to calculate the cooldown, and we don't want reincarnations due to sled evacuation to "count against" the instance's auto-restart cooldown (since they are not the VM's "fault"). That will require us to thread it all the way through to the saga.
• It's probably nice for the saga's logs and stuff to be able to distinguish between sagas that are restarting a failed instance and those that are restarting an evacuated one, for debugging purposes. There's a Reason enum in the instance_start saga that's included in the saga's log messages, and it would be nice if that distinguished between evacuation and failure.
• Determining the state to put the instance back into if the saga unwinds, as @jgallagher mentioned.

Should we skip the cooldown period for VMMs being reincarnated due to evacuation? We don't know how many times any given instance will suffer reincarnation due to evacuation (worst case is "num sleds" times, I guess?), and it doesn't seem like that should induce a cooldown unless the instance is somehow at fault for the reincarnation failing.

Yeah, the cooldown period is only intended to apply to instances that failed. I think we should not count it against evacuated VMMs.

We probably also don't want to change the value of time_last_auto_restarted when we are an restarting an evacuated VMM, because that's how we calculate whether the cooldown has elapsed. I think we probably only want restarts that are due to a failure to set that timestamp, because otherwise, if the VMM is restarted due to an evacuation and then fails, we will count the evacuation against it when determining if the cooldown has elapsed.

Didn't get to this bit yet, will finish tomorrow!

Yeah, the cooldown period is only intended to apply to instances that failed. I think we should not count it against evacuated VMMs.

We probably also don't want to change the value of time_last_auto_restarted when we are an restarting an evacuated VMM, because that's how we calculate whether the cooldown has elapsed. I think we probably only want restarts that are due to a failure to set that timestamp, because otherwise, if the VMM is restarted due to an evacuation and then fails, we will count the evacuation against it when determining if the cooldown has elapsed.

I think this all needs to be ORed with the dsl::state.eq(InstanceState::SledEvacuating), so that the auto-restart cooldown doesn't apply to evacuated instances.

I think the nicest way to do that might be to rewrite the whole query so that there is a separate predicate for failed instances and evacuated instances which get ORed together, and then ANDed with the stuff that applies everywhere ("intended state is running, not deleted, etc). Something like this:

        // Query fragment for selecting *failed* instances that are eligible to
        // restart
        let can_restart_from_failure = (dsl::auto_restart_policy
            .eq(InstanceAutoRestartPolicy::BestEffort)
            // If the auto-restart policy is null, then it should
            // default to "best effort".
            .or(dsl::auto_restart_policy.is_null()))
            // An instance whose last reincarnation was within the cooldown
            // from now must remain in _bardo_ --- the liminal state between 
            // death and rebirth --- before its next reincarnation.
            .and(
                // If the instance has never previously been reincarnated, then
                // it's allowed to reincarnate.
                dsl::time_last_auto_restarted
                    .is_null()
                    // Or, if it has an overridden cooldown period, has that 
                    // elapsed?
                    .or(dsl::auto_restart_cooldown.is_not_null().and(
                        dsl::time_last_auto_restarted
                            .le(now.nullable() - dsl::auto_restart_cooldown),
                    ))
                    // Or, finally, if it does not have an overridden cooldown
                    // period, has the default cooldown period elapsed?
                    .or(dsl::auto_restart_cooldown.is_null().and(
                        dsl::time_last_auto_restarted
                            .le((now - Self::DEFAULT_COOLDOWN).nullable()),
                    )),
            );
        
        // An instance should only be automatically restarted if it is intended
        // to be in the `Running` state. Instances which were requested to stop
        // should not be automatically restarted, even if they have failed.
        dsl::intended_state.eq(IntendedState::Running)
            // Either the failed-instance preduicate from above matches, *or*
            // the instance is rebooting due to its sled evacuating. When an
            // instance had to stop for sled evacuation, can always be
            // reincarnated, regardless of its auto-restart policy and when it
            // last reincarnated.
            .and(
                can_restart_from_failure
                    .or(dsl::state.eq(InstanceState::SledEvacuating))
            )
            // Deleted instances may not be reincarnated.
            .and(dsl::time_deleted.is_null())
            // If the instance is currently in the process of being updated,
            // let's not mess with it for now and try to restart it on another
            // pass.
            .and(dsl::updater_id.is_null())

(note that this is all kind of just a sketch and it...might not even compile)

Yes, I think the ReincarnationReason needs to be passed into the saga. There are a couple other reasons this is necessary:

• As I discussed in https://github.com/oxidecomputer/omicron/pull/11504/changes#r4427704695, we probably don't want to set the time_last_auto_restarted timestamp when the instance-start saga completes, since that will be used to calculate the cooldown, and we don't want reincarnations due to sled evacuation to "count against" the instance's auto-restart cooldown (since they are not the VM's "fault"). That will require us to thread it all the way through to the saga.
• It's probably nice for the saga's logs and stuff to be able to distinguish between sagas that are restarting a failed instance and those that are restarting an evacuated one, for debugging purposes. There's a Reason enum in the instance_start saga that's included in the saga's log messages, and it would be nice if that distinguished between evacuation and failure.
• Determining the state to put the instance back into if the saga unwinds, as @jgallagher mentioned.

Does this need to unwind into SledEvacuating instead of NoVmm if our incoming reason was SledEvacuating?

Makes sense so we can try to restart it, but it also introduces the possibility of a race if the user stops the instance while the saga is unwinding. I'm adding some safeguards here to handle this. Hopefully it's enough.

Mostly this is a question about the other race you found (wondering if we have a similar problem here) - does this commit need to confirm the intended state of the instance hasn't changed to Stopped in the meantime? IIUC from the other race, intended state changes don't bump the internal state generation numbers, so this commit could probably go through and restart an instance that had already been successfully stopped?

Just to keep this comment with updated information. We spoke about it IRL and yes, this could also be a race. Will be handled in an upcoming PR

Should we tweak this to include evacuated instances? (There are similar docs in other places w.r.t. this task, like

take it or leave it: if the intent is to prevent these tasks from being periodically activated during the test, we might wanna make their period Duration::MAX rather than one day? the test probably won't get stuck running for an entire day, but...

hmm, this is kinda uncomfy. the only reason this would fail IIUC is if either the instance is deleted, or if there's a transient database connection error, right? if it's deleted, we could just give up entirely, but if there's an error reading the db record, either it's transient and could be retried (allowing us to get the correct state) OR the subsequent write back to the instance state right after this will also fail, because the database is down, and then the saga will be stuck. maybe the whole thing ought to be wrapped in a retry loop so that we keep trying if we are temporarily unable to talk to CRDB, instead of guessing about the state?

take it or leave it, but personally, I really don't care for tests that construct a bunch of cases like this and then loop over them. If it were me, I'd much rather we do this by having a helper function that runs one Case, and then have separate test functions for each test case that call the helper function and pass in the desired input configuration. The advantages are that (1) cargo nextest will run all the tests in parallel, instead of sequentially, and (2) we can see which individual test case failed, and if one runs with --no-fail-fast, one can see that other test cases have succeeded.

Ug, I don't think into_backoff_error() is correct here, although through no fault of its own. into_backoff_error() returns a permanent error for anything that isn't retryable, and the only error that is retryable is ServiceUnavailable:

However, database errors are turned into InternalError, not ServiceUnavailable:

That combination seems wrong - it means we don't consider database errors as retryable, which means a transient db issue here turns into a stuck saga, I think?

I don't really know what to do about all of this. Failing in an undo node is Really Bad, right?

Ugh :(

For what it's worth, before this change, the sis_move_to_starting_undo was already returning all errors from instance_update_runtime. So in a sense this somewhat of an improvement because it's at least retrying on ServiceUnavailable?

Perhaps we can open up a separate issue to improve how db errors are reported? retryable() is used in other places, and it's more correct to have the DB to correctly report errors. As a separate PR we can change public_error_from_diesel and public_error_from_diesel_lookup to map transient diesel or cockroachdb errors to ServiceUnavailable. What do you think?

For what it's worth, before this change, the sis_move_to_starting_undo was already returning all errors from instance_update_runtime

Yeah, I think that behavior was wrong, and we should almost certainly change it. I will accept that this isn't the place to do it, though.

Does the fact that we don't enforce the cooldown period for evacuating restarts mean this activate() could turn into a "spam reincarnations as fast as possible" if there's an instance that's failing to reincarnate? Say there's some problem with dendrite that's causing reincarnation to fail; I think we'd do something like:

1. kick off instance start saga for an instance we're trying to reincarnate due to sled evacuation
2. it goes through placement and constructs a VMM record on a sled with capacity
3. we get to dpd_ensure, which fails
4. the saga unwinds; we get here, this immediately activates the reincarnation task and we loop back around to 1

Maybe this sequence is already a problem for other instance restarts that are failing too, I'm not sure. I guess it depends on when we update time_last_auto_restarted, which would affect what can_reincarnate() returns here?

😖 Yeah, you're right.

I think one way to mitigate this would be to change sis_move_to_starting_undo to write Failed instead of SledEvacuating if the intended state is still Running, when the restart unwinds. We can then force that instance into the cooldown period.

The caveat here, is again the auto-restart policy. If the user set it to Never it'll remain stopped, but then again I don't think that's terribly bad?

Thoughts?

I could see cases for any of these:

1. Move it to Failed (as in [live-update] Modify instance reincarnation to account for sled evacuations #11504 (comment), this doesn't seem right but I agree the practical impact is probably pretty small)
2. If our Reason is SledEvacuating, just don't activate the bg task, and rely on its normal timer
3. Do Distinguish between instance cooldown after reincarnation, and time of last auto restart #11707 and then impose some nominal cooldown period based on that timestamp (this can be much shorter than 5 minutes and doesn't need to increase over time; just something to avoid this turning into a hot loop)

I think I have a mild preference for 2? It feels a little yucky and would need a sort of apologetic comment explaining it, but I think it's easy to implement and the reincarnation bg task already runs every minute, so in practice it's pretty similar to picking option 3 with a 1 minute cooldown.

Yeah, I'm personally fine with either of 2 or 3 but 2 seems the simplest.

Oof, I'm worried about yet another race here (but this one is specific to sled evacuation):

• Once we get to sis_move_to_starting, the VMM row is in the Starting state
• If anything goes wrong later in in the saga, we'll unwind, and this undo node is supposed to put it back in the SledEvacuating state, conditional on the generation number not having changed

However, what if something else marks the VMM as Failed while this saga is running? That would bump the generation number, which would prevent us from putting it back in SledEvacuating, which means an instance that we were trying to reincarnate due to sled evacuation is now stuck in Failed instead of SledEvacuating. That's maybe not the end of the world? But I'm worried there's no obvious / easy fix here - explicitly overwriting a Failed state here seems awfully precarious. Should we have modeled sled evacuation as a separate column instead of a stat?

In terms of "how could it be marked failed in between sis_move_to_starting and the end of the saga", I think either of these are viable paths:

• sled-agent fails to start the propolis zone, publishes that state to Nexus, which runs an instance_update saga that marks it failed
• any time we make it past sis_ensure_registered, the instance watcher bg task could conceivably poll sled-agent for the VMM status. If we then fail after sis_ensure_registered but before the end, start unwinding, if the instance-watcher polls sled agent anywhere between sis_ensure_unregistered and here, it'll see that the VMM is gone and mark it as failed

However, what if something else marks the VMM as Failed while this saga is running? That would bump the generation number, which would prevent us from putting it back in SledEvacuating, which means an instance that we were trying to reincarnate due to sled evacuation is now stuck in Failed instead of SledEvacuating. That's maybe not the end of the world?

I don't think it's a huge issue, since the reincarnation task attempts to reincarnate instances in Failed state as well. There's just the caveat that if they have an auto-restart policy of Never, they won't be restarted, but then that was a conscious choice from the user no?

There'll be an added cooldown period for the Failed instances but that would only make the update take longer. So not a huge deal?

We can also open up another issue to look into this race as well as a follow up item.

• any time we make it past sis_ensure_registered, the instance watcher bg task could conceivably poll sled-agent for the VMM status. If we then fail after sis_ensure_registered but before the end, start unwinding, if the instance-watcher polls sled agent anywhere between sis_ensure_unregistered and here, it'll see that the VMM is gone and mark it as failed

FWIW, this part, at least, should no longer be the case after #11696, which changes the unwinding saga to immediately change the VMM state to one that will not be polled by the instance-watcher while it is being unregistered. I think this is a bug across the board that isn't actually specific to the reincarnation changes here.

I don't think it's a huge issue, since the reincarnation task attempts to reincarnate instances in Failed state as well. There's just the caveat that if they have an auto-restart policy of Never, they won't be restarted, but then that was a conscious choice from the user no?

Yeah, I don't think this is a blocker, but I do think it's still incorrect and worth fixing at some point, if the design is that a SledEvacuating instance should continue to be reincarnated until success or it's explicitly stopped. I'm on board with filing an issue (or noting this on one of the existing ones - this is maybe just another flavor of #11706?).

Ugh :(

For what it's worth, before this change, the sis_move_to_starting_undo was already returning all errors from instance_update_runtime. So in a sense this somewhat of an improvement because it's at least retrying on ServiceUnavailable?

Perhaps we can open up a separate issue to improve how db errors are reported? retryable() is used in other places, and it's more correct to have the DB to correctly report errors. As a separate PR we can change public_error_from_diesel and public_error_from_diesel_lookup to map transient diesel or cockroachdb errors to ServiceUnavailable. What do you think?

However, what if something else marks the VMM as Failed while this saga is running? That would bump the generation number, which would prevent us from putting it back in SledEvacuating, which means an instance that we were trying to reincarnate due to sled evacuation is now stuck in Failed instead of SledEvacuating. That's maybe not the end of the world?

I don't think it's a huge issue, since the reincarnation task attempts to reincarnate instances in Failed state as well. There's just the caveat that if they have an auto-restart policy of Never, they won't be restarted, but then that was a conscious choice from the user no?

There'll be an added cooldown period for the Failed instances but that would only make the update take longer. So not a huge deal?

We can also open up another issue to look into this race as well as a follow up item.